Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Moving hooks on thread #205

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

GeoffreyBooth
Copy link
Member

Following up #200 (comment), this is a draft proposal for option 3 from #201. This builds on #198 and assumes that that proposal would be implemented first.

@GeoffreyBooth GeoffreyBooth added documentation Improvements or additions to documentation loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team labels Jun 22, 2024
@GeoffreyBooth

This comment was marked as resolved.

Copy link

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great start, although would be nice to see a little more design around the resolve and load caller APIs here.

doc/design/proposal-moving-hooks-on-thread.md Outdated Show resolved Hide resolved
doc/design/proposal-moving-hooks-on-thread.md Outdated Show resolved Hide resolved
GeoffreyBooth and others added 2 commits June 23, 2024 15:02
@ghost
Copy link

ghost commented Jul 14, 2024

Some support for this.

My loader is an import interceptor. The main use case is full mocks in tests. On the principle "don't hide errors", I compare the given mock to the shape of original module. The attempt fails if the mock tries to add a name that doesn't exist, omit a name that does exist, or set a value for a name that's ambiguous. If the mock does any of these things then the test passes while production fails.

// util.mjs
export function frizzle () {}
// dazzle.mjs
import { frizzle, frazzle } from './util.mjs'
export class Dazzle { /* use frizzle and frazzle */ }
// test.mjs
const frizzle = sinon.stub()
const frazzle = sinon.stub() // not present in the authentic module
pose('./util.mjs', { frizzle, frazzle })
const { Dazzle } = await import('./dazzle.mjs')
// PhantomExport: Attempted to add an absent name to the module

Achieving this requires analyzing the full module graph. A naive approach would simply load each module but I'd prefer to avoid that. Where the entire module is mocked no exports can ever be called so instantiating is a waste, it may take significant time if it fronts a large graph of eg 100 modules, and there may load time modification of state that I'd like to isolate the test from.

I saw an APM rep in one of the meetings say they really need to load the authentic module so they can wrap it with tracing functions. Just want to register here that I kind of have the opposite use case. I'd specifically like to analyze all the source without evaluating it.

This hook enables exactly these things, so I'm in strong support.

@joyeecheung
Copy link
Member

I am not sure if I understand the use case correctly but sounds like it can be addressed with the link hook proposed in #198, which is invoked after module compilation and before evaluation, at that point you have the real export names parsed by V8, but the modules are not evaluated yet so you can perform the tests there and if it throws, the graph would not be evaluated at all. That also saves you the cost of parsing the modules to get the export names by yourself, since you can piggypack on the names readily parsed by V8.

@ghost
Copy link

ghost commented Aug 5, 2024

It would be awesome, that's definitely the most complicated piece. It's really close. I'd need the list of ambiguous names to fully replace it. Would really prefer to let the engine handle this.

@ghost
Copy link

ghost commented Aug 5, 2024

It's seeming better and better the more I mull. I misunderstood link at first as posteval precache, giving access to the real exports but allowing last minute override. But actually it's just the shape and allows preeval override. It's perfect.

It will work without the ambiguous names, that's really just a special case of undefined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants